Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(conf): remove CNAME from default dns_order option #13107

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

chobits
Copy link
Contributor

@chobits chobits commented May 29, 2024

Summary

DNS servers are capable of performing recursive lookups on behalf of clients and DNS client could directly extract IP addresses from RRs due to #13002. Consequently, clients often don't need to query CNAME records.

The impact on customers is that when they use the default dns_order option without explicit configuration, their local DNS servers won't get any CNAME requests, and there won't be CNAME dereferencing for those queries.

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

Fix KAG-4606

@github-actions github-actions bot added core/templates cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels May 29, 2024
@chobits chobits force-pushed the fix/dns_order-remove-cname branch from a069f1b to 016d19a Compare May 29, 2024 10:00
@chobits
Copy link
Contributor Author

chobits commented May 29, 2024

This PR removes Kong's DNS client's handling of CNAME. As a result, the logic for CNAME dereferencing and recursive loop detection becomes unnecessary and can be removed. However, removing them requires some effort, so for now, they're left as is.

@Tieske
Copy link
Member

Tieske commented May 29, 2024

isn't this a breaking change?

@chobits
Copy link
Contributor Author

chobits commented May 30, 2024

isn't this a breaking change?

I reconsidered it, and changing the default behavior is a breaking change. I'll modify the type in the changelog.

However, users can actually be unaware of this behavior, because CNAME can still be configured in dns_order. This PR does not remove the CNAME-processing logic from DNS client. This means that customers can seamlessly upgrade to this version of the PR, whether they are using the default dns_order or have explicitly configured dns_order with CNAME, such as (dns_order=LAST,A,CNAME).

What users will notice is that when they use the default dns_order option without explicit configuration, their local DNS servers will not receive any CNAME requests and there is no CNAME dereferencing for that query.

@chobits chobits force-pushed the fix/dns_order-remove-cname branch 2 times, most recently from 9ffaed6 to 401c08b Compare May 30, 2024 08:35
@chobits chobits force-pushed the fix/dns_order-remove-cname branch from dfb0891 to 8e7a60c Compare May 31, 2024 08:18
@chobits chobits force-pushed the fix/dns_order-remove-cname branch from cf37975 to c34179f Compare June 3, 2024 02:05
@@ -108,7 +108,7 @@ for _, strategy in helpers.each_strategy() do

local service = bp.services:insert {
name = "tests-retries",
host = "nowthisdoesnotexistatall",
host = "nowthisdoesnotexistatall.test",
Copy link
Contributor Author

@chobits chobits Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NOTE: The AWS route 53 DNS server will reply "(2) server failure" for an A-type domain without dot, like "nowthisdoesnotexistatall".
Because we have removed CNAME from LAST_ORDER, the final attempt is now changed from CNAME to A.

@chobits chobits marked this pull request as draft June 24, 2024 07:09
@chobits
Copy link
Contributor Author

chobits commented Jun 24, 2024

hold it , because #13002 has been reverted. We need to fix the regression of #13002, then we could merge this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee core/templates size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants